Skip to content

Incremental port #1322

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 39 commits into
base: main
Choose a base branch
from
Open

Incremental port #1322

wants to merge 39 commits into from

Conversation

sheetalkamat
Copy link
Member

No description provided.

@sheetalkamat sheetalkamat marked this pull request as ready for review July 21, 2025 22:38
@Copilot Copilot AI review requested due to automatic review settings July 21, 2025 22:38
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR continues the incremental port of the TypeScript compiler with baseline test updates reflecting changes to output formatting, command-line invocation, and internal processing. The changes primarily update test baselines to match the new compiler's behavior patterns.

Key changes:

  • Standardized test output formatting to use tsgo command instead of tsc
  • Updated baseline outputs to include library references and diagnostic information
  • Removed references to deprecated outFile option and related configuration errors

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seemingly still logical races or something; note the CI failure:

Using D:\a\typescript-go\typescript-go\Herebyfile.mjs to run baseline-accept
Starting baseline-accept
Finished baseline-accept in 19ms
Completed baseline-accept in 20ms
diff --git a/testdata/baselines/reference/tsc/incremental/when-global-file-is-added,-the-signatures-are-updated.js b/testdata/baselines/reference/tsc/incremental/when-global-file-is-added,-the-signatures-are-updated.js
index e3612043..f4adbdf5 100644
--- a/testdata/baselines/reference/tsc/incremental/when-global-file-is-added,-the-signatures-are-updated.js
+++ b/testdata/baselines/reference/tsc/incremental/when-global-file-is-added,-the-signatures-are-updated.js
@@ -551,7 +551,6 @@ declare function something2(): number;
 //// [/home/src/workspaces/project/src/fileNotFound.js] *new* 
 function something2() { return 20; }
 
-//// [/home/src/workspaces/project/src/main.js] *modified time*
 //// [/home/src/workspaces/project/tsconfig.tsbuildinfo] *modified* 
 {"version":"FakeTSVersion","fileNames":["../../tslibs/TS/Lib/lib.d.ts","./src/filePresent.ts","./src/fileNotFound.ts","./src/anotherFileWithSameReferenes.ts","./src/newFile.ts","./src/main.ts"],"fileInfos":[{"version":"7dee939514de4bde7a51760a39e2b3bfa068bfc4a2939e1dbad2bfdf2dc4662e-/// \u003creference no-default-lib=\"true\"/\u003e\ninterface Boolean {}\ninterface Function {}\ninterface CallableFunction {}\ninterface NewableFunction {}\ninterface IArguments {}\ninterface Number { toExponential: any; }\ninterface Object {}\ninterface RegExp {}\ninterface String { charAt: any; }\ninterface Array\u003cT\u003e { length: number; [n: number]: T; }\ninterface ReadonlyArray\u003cT\u003e {}\ninterface SymbolConstructor {\n    (desc?: string | number): symbol;\n    for(name: string): symbol;\n    readonly toStringTag: symbol;\n}\ndeclare var Symbol: SymbolConstructor;\ninterface Symbol {\n    readonly [Symbol.toStringTag]: string;\n}\ndeclare const console: { log(msg: any): void; };","affectsGlobalScope":true,"impliedNodeFormat":1},{"version":"8f597c315d3bba69a79551601042fdcfe05d35f763762db4908b255c7f17c7d5-function something() { return 10; }","signature":"4f3eeb0c183707d474ecb20d55e49f78d8a3fa3ac388d3b7a318d603ad8478c2-declare function something(): number;\n","affectsGlobalScope":true,"impliedNodeFormat":1},{"version":"6c5b229cbf53b2c6867ab14b139eeac37ed3ec0c1564ba561f7faa869aaba32c-function something2() { return 20; }","signature":"14ba6a62cd6d3e47b343358b2c3e1b7e34b488b489f0d9b915c796cd2e61bbad-declare function something2(): number;\n","affectsGlobalScope":true,"impliedNodeFormat":1},{"version":"552b902790cfbb88a7ed6a7b04b24bb18c58a7f52bcfe7808912e126359e258d-/// \u003creference path=\"./filePresent.ts\"/\u003e\n/// \u003creference path=\"./fileNotFound.ts\"/\u003e\nfunction anotherFileWithSameReferenes() { }","signature":"d4dbe375786b0b36d5425a70f140bbb3d377883027d2fa29aa022a2bd446fbda-declare function anotherFileWithSameReferenes(): void;\n","affectsGlobalScope":true,"impliedNodeFormat":1},{"version":"a4c88e3619994da0f5e4da2dc210f6038e710b9bb831003767da68c882137fb1-function foo() { return 20; }","signature":"f0d67d5e01f8fff5f52028627fc0fb5a78b24df03e482ddac513fa1f873934ee-declare function foo(): number;\n","affectsGlobalScope":true,"impliedNodeFormat":1},{"version":"930f3c7023d87506648db3c352e86f7a2baf38e91f743773fddf4a520aff393a-/// \u003creference path=\"./newFile.ts\"/\u003e\n/// \u003creference path=\"./filePresent.ts\"/\u003e\n/// \u003creference path=\"./fileNotFound.ts\"/\u003e\nfunction main() { }something();something();foo();","signature":"ed4b087ea2a2e4a58647864cf512c7534210bfc2f9d236a2f9ed5245cf7a0896-declare function main(): void;\n","affectsGlobalScope":true,"impliedNodeFormat":1}],"fileIdsList":[[2,3],[2,3,5]],"options":{"composite":true},"referencedMap":[[4,1],[6,2]],"latestChangedDtsFile":"./src/fileNotFound.d.ts"}
 //// [/home/src/workspaces/project/tsconfig.tsbuildinfo.readable.baseline.txt] *modified* 

@@ -21,6 +21,7 @@ type Diagnostic struct {
relatedInformation []*Diagnostic
reportsUnnecessary bool
reportsDeprecated bool
skippedOnNoEmit bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do the other functions that compare/do equality need to be updated to check this?

Comment on lines +68 to +75
if s.Len() != other.Len() {
return false
}
for key := range s.M {
if !other.Has(key) {
return false
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could probably just use maps.Equal(s.M, other.M).

})
}

func (s *SyncSet[T]) ToArray() []T {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say ToSlice() here, just since it's not actually a Go array.

@@ -462,3 +469,93 @@ func getDeclarationDiagnostics(host EmitHost, file *ast.SourceFile) []*ast.Diagn
transform.TransformSourceFile(file)
return transform.GetDiagnostics()
}

type AnyProgram interface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better name for this than "AnyProgram"?

@@ -72,6 +77,9 @@ func (h *compilerHost) GetSourceFile(opts ast.SourceFileParseOptions) *ast.Sourc
}

func (h *compilerHost) GetResolvedProjectReference(fileName string, path tspath.Path) *tsoptions.ParsedCommandLine {
commandLine, _ := tsoptions.GetParsedCommandLineOfConfigFilePath(fileName, path, nil, h, nil)
if h.extendedConfigCache == nil {
h.extendedConfigCache = &collections.SyncMap[tspath.Path, *tsoptions.ExtendedConfigCacheEntry]{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it is concurrency unsafe. But, the coverage never says this happens, so maybe this should just panic?

@@ -41,25 +41,20 @@ func applyBulkEdits(text string, edits []core.TextChange) string {
return b.String()
}

func CommandLine(sys System, cb cbType, commandLineArgs []string) ExitStatus {
func CommandLine(sys System, commandLineArgs []string, testing bool) (ExitStatus, *incremental.Program, *Watcher) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it time to start returning a struct here? I assume that this is all just here because testing needs more info out of the CommandLine function?

// Options diagnostics include global diagnostics (even though we collect them separately),
// and global diagnostics create checkers, which then bind all of the files. Do this binding
// early so we can track the time.
bindStart := recordTime("bind", true, time.Time{})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how I feel about this pattern... Feels a bit weird for the emitter file to be the one with this time tracking code, and then it be indiredcted through callbacks and so on, when it was quite direct before. What was wrong with the old way? Can this just return a function closure that is used to start/stop the range?

func emitAndReportStatistics(
sys System,
program compiler.AnyProgram,
getCoreProgram func() *compiler.Program,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this indirection needed? Can't just be immediately invoked? Or just use program?

Comment on lines +220 to +223
if h.program.updatedSignatureKinds != nil {
h.program.updatedSignatureKinds.Store(file, SignatureUpdateKindStoredAtEmit)
}
h.program.snapshot.buildInfoEmitPending = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't gotten there yet, but it strikes me as odd that we are atomically mutating one thing at the same time as we are not atomically mutating another thing, when both are sort of "on" the same Program... I can't really put my finger on it but it gives me the bad feeling of the model not being consistent and free of logical races.

@@ -217,6 +217,11 @@ var targetToLibMap = map[core.ScriptTarget]string{
core.ScriptTargetES2015: "lib.es6.d.ts", // We don't use lib.es2015.full.d.ts due to breaking change.
}

// Used for test to add the files
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just delete this; I initially read this as it saying that a test was modifying the map, when it's just adding fake files.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That being said, why do we need this when the harness already knows how to do this without this mapping?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants